Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(react): Add versioning for workspace libraries #19063

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

ndcunningham
Copy link
Contributor

@ndcunningham ndcunningham commented Sep 7, 2023

This PR grants users the ability to add versioning for their workspace libraries.

At build time inside withModuleFederation get each workspace library's version by:

  1. Looking at the parent's package.json to get the version range
  2. If not found in the root we look at the library's package.json to get the fixed version
  3. If neither is found, we do not do any versioning and the functionality remains as is currently.

If the need arises where you do not want this functionality (since it is the current default) you can override it by using the shared API inside of the module-federation.config.js.

When your code is built you can check the build artifact's remoteEntry.js for your host/remote and you should see your workspace library registered with a version.

@ndcunningham ndcunningham self-assigned this Sep 7, 2023
@vercel
Copy link

vercel bot commented Sep 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Sep 13, 2023 7:50pm

@Coly010
Copy link
Contributor

Coly010 commented Sep 8, 2023

I'm not sure this is the correct approach.

When we run the build of a remote or host, the ModuleFederationPlugin will read the package.json of the workspace libraries, if it exists, and set the version in its register function to that version. If it doesn't exist it sets it to 0.0.0.

We do set requiredVersion: false in the shareWorkspaceLibraries config. Instead, we should potentially switch this to be

requiredVersion: workspaceLib.packageJson?.version ?? false

Then the user can define a range of versions of the library that the app is compatible with in their shared function in the module-federation.config if they desire, otherwise, we'll have it pinned to the version of the library at the time the app was built:

shared: (libraryName, libraryConfig) => {
  if(libraryName === '@acme/mylib') {
     return {
         ...libraryConfig,
        requiredVersion: "~1.0.0"
  } 
}

If their workspace library does not have a package.json file, they can add one with just "version": "1.0.0" .

I tested this out locally, and it does work. If the remote and the host have different versions set in their shared function, then they just use their own version, otherwise, it uses a shared version between them.

This should also reduce the need of an additional property in the config.

@ndcunningham ndcunningham force-pushed the feat/module-federation-lib-version branch from f3c2932 to 78f1c8d Compare September 8, 2023 16:55
@ndcunningham ndcunningham force-pushed the feat/module-federation-lib-version branch 4 times, most recently from e323d61 to 95e1e99 Compare September 11, 2023 21:29
@ndcunningham ndcunningham marked this pull request as ready for review September 11, 2023 21:30
@@ -371,6 +373,80 @@ describe('MF Share Utils', () => {
).not.toThrow();
});
});

it('should using shared library version from root package.json if available', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a clean-up item later, but it'd be nice to use either a real fs, or mem-fs here. Spying and mocking like this couples the test to the implementation too much. For example, if existsSync and realTsPathMappings are replaced with something else we'd have to update unit tests.

mem-fs can be used like:

import 'nx/src/utils/testing/mock-fs';
import { vol } from 'memfs';

// ...
vol.fromJson({
  'tsconfig.base.json': { ... },
  // ...
}, '/');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a cleanup task. If we are changing over to use this it would make sense to update all the unit test inside share.spec.ts

@ndcunningham ndcunningham force-pushed the feat/module-federation-lib-version branch 2 times, most recently from 65bcdf7 to 7fe6843 Compare September 12, 2023 21:17
@ndcunningham ndcunningham force-pushed the feat/module-federation-lib-version branch from 7fe6843 to 3f8600a Compare September 12, 2023 21:20
@ndcunningham ndcunningham force-pushed the feat/module-federation-lib-version branch from 3f8600a to 77f7f8f Compare September 13, 2023 19:47
@ndcunningham ndcunningham force-pushed the feat/module-federation-lib-version branch from 77f7f8f to 240d0a2 Compare September 13, 2023 19:49
Copy link
Contributor

@Coly010 Coly010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉

in a follow up task we should add or change the e2e to build the shell, then change the version of the lib, then build the remote.

then we should read the main.js file of the shell to find the version the library is being registered with by MFP, and check it is at the earlier version, and then we should look for the same line in the remote’s remoteEntry.mjs and check its version matches the new version.

@jaysoo jaysoo merged commit 0a7efc6 into nrwl:master Sep 14, 2023
3 checks passed
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
@ndcunningham ndcunningham deleted the feat/module-federation-lib-version branch October 10, 2023 18:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants